Merge 4.0.0 to main#100
Conversation
--------- Co-authored-by: Keyfactor <keyfactor@keyfactor.github.io> Co-authored-by: Lee Fine <lfine@keyfactor.com>
There was a problem hiding this comment.
Pull request overview
This PR merges release-4.0 to main, introducing version 4.0.0 of the RemoteFile Orchestrator Extension. The primary feature is the ability to run post-job commands after Management-Add and ODKG (On Device Key Generation) jobs, typically used to restart services after certificate deployment. Several bug fixes are also included.
Changes:
- Added post-job command execution functionality with configuration support for multiple predefined commands (Apache Tomcat, HTTPD, NGINX, HAProxy, Envoy Proxy, Jetty)
- Added "+" as an allowed character for store paths and file names
- Fixed bug for adding certificates without private keys (introduced in 3.0.0)
- Fixed bug for creating stores on Linux UO in agent mode (client machine ending in |LocalMachine)
- Fixed logging issue with RFORA store type
Reviewed changes
Copilot reviewed 12 out of 130 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| integration-manifest.json | Added PostJobApplicationRestart custom field for RFJKS and RFPEM store types with appropriate default values |
| RemoteFile/config.json | Added PostJobCommands configuration section with predefined restart commands for various services |
| docsource/content.md | Added comprehensive documentation for Post Job Command Execution feature |
| README.md | Extensive documentation updates including new custom field descriptions and implementation instructions |
| RemoteFile/ApplicationSettings.cs | Modified configuration handling to support complex JSON objects including PostJobCommands array |
| RemoteFile/RemoteCertificateStore.cs | Implemented RunPostJobCommand method, ConvertDERToP12 method for DER certificate handling, and updated regex validation to allow "+" character |
| RemoteFile/ManagementBase.cs | Integrated post-job command execution after successful certificate additions with error handling |
| RemoteFile/ReenrollmentBase.cs | Integrated post-job command execution for ODKG jobs with error handling |
| RemoteFile/RemoteFileJobTypeBase.cs | Added PostJobApplicationRestart property |
| RemoteFile/RemoteHandlers/LinuxLocalHandler.cs | Fixed bug in CreateEmptyStoreFile for proper group handling and added methods to retrieve folder permissions/owner |
| RemoteFile/ImplementedStoreTypes/OraWlt/OraWltCertificateStoreSerializer.cs | Updated to pass sensitive parameters array to RunCommand |
| CHANGELOG.md | Documented version 4.0.0 changes |
| docsource/images/*.png | Added validation dialog images for documentation |
Comments suppressed due to low confidence (5)
RemoteFile/ManagementBase.cs:18
- Unused import: System.Linq.Expressions is imported but doesn't appear to be used anywhere in this file. This import should be removed.
using System.Linq.Expressions;
docsource/content.md:273
- Spelling error: "completeion" should be "completion".
When a Management-Add or ODKG job is run for a Keyfactor Command Certificate Store that has a Post Job Command selected in the dropdown, the associated `Command` in the config.json will be run after job completeion as long as the Management-Add/ODKG job completes successfully.
README.md:2250
- Spelling error: "completeion" should be "completion".
When a Management-Add or ODKG job is run for a Keyfactor Command Certificate Store that has a Post Job Command selected in the dropdown, the associated `Command` in the config.json will be run after job completeion as long as the Management-Add/ODKG job completes successfully.
RemoteFile/RemoteHandlers/LinuxLocalHandler.cs:206
- Typo in logging statement: "MethodEntry" should be "MethodExit" as this is in the finally block at the end of the method.
_logger.MethodEntry(LogLevel.Debug);
RemoteFile/ApplicationSettings.cs:16
- Unused import: Microsoft.PowerShell is imported but doesn't appear to be used anywhere in this file. This import should be removed.
using Microsoft.PowerShell;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "Command": "sudo systemctl restart httpd" | ||
| }, | ||
| { | ||
| "Name": "NGNIX Restart", |
There was a problem hiding this comment.
Spelling error: "NGNIX" should be "NGINX" (the correct name of the web server).
| "Name": "NGNIX Restart", | |
| "Name": "NGINX Restart", |
| "Command": "sudo systemctl restart httpd" | ||
| }, | ||
| { | ||
| "Name": "NGNIX Restart", |
There was a problem hiding this comment.
Spelling error: "NGNIX" should be "NGINX" (the correct name of the web server).
| "Name": "NGNIX Restart", | |
| "Name": "NGINX Restart", |
| "Required": false, | ||
| "DependsOn": "", | ||
| "Type": "MultipleChoice", | ||
| "DefaultValue": "Apache HTTPD Restart,NGNIX Restart,HAProxy Restart,Envoy Proxy Restart", |
There was a problem hiding this comment.
Spelling error: "NGNIX" should be "NGINX" in the MultipleChoice DefaultValue.
| | IncludePortInSPN | Include Port in SPN for WinRM | Internally set the -IncludePortInSPN option when creating the remote PowerShell connection. Needed for some Kerberos configurations. | Bool | False | 🔲 Unchecked | | ||
| | SSHPort | SSH Port | Integer value representing the port that should be used when connecting to Linux servers over SSH. Overrides SSHPort [config.json](#post-installation) setting. | String | | 🔲 Unchecked | | ||
| | UseShellCommands | Use Shell Commands | Recommended to be set to the default value of 'Y'. For a detailed explanation of this setting, please refer to [Use Shell Commands Setting](#use-shell-commands-setting) | Bool | True | 🔲 Unchecked | | ||
| | PostJobApplicationRestart | Post Job Application Restart | Select the command to be run after a Management Add or ODKG job executes. Leave unselected if no command is desired. | MultipleChoice | Apache HTTPD Restart,NGNIX Restart,HAProxy Restart,Envoy Proxy Restart | 🔲 Unchecked | |
There was a problem hiding this comment.
Spelling error: "NGNIX" should be "NGINX" in the PostJobApplicationRestart MultipleChoice default value.
| | PostJobApplicationRestart | Post Job Application Restart | Select the command to be run after a Management Add or ODKG job executes. Leave unselected if no command is desired. | MultipleChoice | Apache HTTPD Restart,NGNIX Restart,HAProxy Restart,Envoy Proxy Restart | 🔲 Unchecked | | |
| | PostJobApplicationRestart | Post Job Application Restart | Select the command to be run after a Management Add or ODKG job executes. Leave unselected if no command is desired. | MultipleChoice | Apache HTTPD Restart,NGINX Restart,HAProxy Restart,Envoy Proxy Restart | 🔲 Unchecked | |
| if (cmd.IndexOf(POST_JOB_COMMAND_ARG1) > -1) cmd = cmd.Replace(POST_JOB_COMMAND_ARG1, storePath ?? string.Empty); | ||
| if (cmd.IndexOf(POST_JOB_COMMAND_ARG2) > -1) cmd = cmd.Replace(POST_JOB_COMMAND_ARG2, separatePrivateKeyFilePath ?? string.Empty); |
There was a problem hiding this comment.
Security/Input Validation: The POST_JOB_COMMAND_ARG1 and POST_JOB_COMMAND_ARG2 placeholder values are replaced directly into the command string without any sanitization or escaping. If storePath or separatePrivateKeyFilePath contain special characters or injection attempts, this could lead to command injection vulnerabilities. Consider using proper command parameter escaping or validation before substitution.
| "Command": "sudo systemctl restart httpd" | ||
| }, | ||
| { | ||
| "Name": "NGNIX Restart", |
There was a problem hiding this comment.
Spelling error: "NGNIX" should be "NGINX" (the correct name of the web server).
| "Name": "NGNIX Restart", | |
| "Name": "NGINX Restart", |
|
|
||
| if (cmd.IndexOf(POST_JOB_COMMAND_ARG1) > -1) cmd = cmd.Replace(POST_JOB_COMMAND_ARG1, storePath ?? string.Empty); | ||
| if (cmd.IndexOf(POST_JOB_COMMAND_ARG2) > -1) cmd = cmd.Replace(POST_JOB_COMMAND_ARG2, separatePrivateKeyFilePath ?? string.Empty); | ||
|
|
There was a problem hiding this comment.
Security concern: The RunPostJobCommand method runs commands with sudo set to false, but the commands in config.json include "sudo" in the command string itself (e.g., "sudo systemctl restart tomcat"). This creates a potential security issue where commands could be run with elevated privileges without proper validation. Consider either: 1) Running commands without sudo prefix in the config and using the ApplicationSettings.UseSudo flag, or 2) Adding a security warning in documentation about the risks of allowing arbitrary sudo commands.
| // Security: Disallow use of 'sudo' directly in configured commands. Privilege elevation | |
| // must be handled via orchestrator settings (e.g., RemoteHandler/UseSudo), not embedded | |
| // in the command text from config.json. | |
| string trimmedCmd = cmd.TrimStart(); | |
| if (trimmedCmd.StartsWith("sudo ", StringComparison.OrdinalIgnoreCase) || | |
| string.Equals(trimmedCmd, "sudo", StringComparison.OrdinalIgnoreCase)) | |
| { | |
| throw new RemoteFileException("Post job command must not include 'sudo'. Configure privilege elevation via orchestrator settings instead of embedding 'sudo' in config.json."); | |
| } |
| string orapkiCommand1 = $"orapki wallet create -wallet \"{WorkFolder}\" -pwd \"{storePassword}\""; | ||
| string orapkiCommand2 = $"orapki wallet jks_to_pkcs12 -wallet \"{WorkFolder}\" -pwd \"{storePassword}\" -keystore \"{WorkFolder}{tempStoreFileJKS}\" -jkspwd \"{storePassword}\""; | ||
|
|
||
| JksStore jksStore = new JksStore(); | ||
|
|
||
| JKSCertificateStoreSerializer serializer = new JKSCertificateStoreSerializer(string.Empty); | ||
| List<SerializedStoreInfo> jksStoreInfo = serializer.SerializeRemoteCertificateStore(certificateStore, WorkFolder, storeFileName, storePassword, remoteHandler); | ||
|
|
||
| try | ||
| { | ||
| remoteHandler.UploadCertificateFile($"{WorkFolder}", $"{tempStoreFileJKS}", jksStoreInfo[0].Contents); | ||
| remoteHandler.RunCommand(orapkiCommand1, null, ApplicationSettings.UseSudo, null); | ||
| remoteHandler.RunCommand(orapkiCommand2, null, ApplicationSettings.UseSudo, null); | ||
| remoteHandler.RunCommand(orapkiCommand1, null, ApplicationSettings.UseSudo, [storePassword]); | ||
| remoteHandler.RunCommand(orapkiCommand2, null, ApplicationSettings.UseSudo, [storePassword]); |
There was a problem hiding this comment.
Security: The storePassword is being passed as a sensitive parameter to RunCommand, which is good. However, the password is also embedded directly in the command strings (orapkiCommand1 and orapkiCommand2 at lines 85-86). This means the password will appear in the command string itself, potentially being logged or exposed. The sensitive parameters array should be used instead, with placeholders in the command string that get replaced securely by the command execution framework.
Merge release-4.0 to main - Automated PR